Add a dockerignore #1962
Add a dockerignore #1962
Conversation
| GOARCH=$TARGETARCH \ | ||
| CGO_ENABLED=$CGO_ENABLED \ | ||
| BPF2GO_CFLAGS=$BPF2GO_CFLAGS \ | ||
| go generate ./... \ | ||
| && go build -o otel-go-instrumentation ./cli/... |
There was a problem hiding this comment.
I think I prefer calling the makefile here,
The main reason being we don't need to duplicate the env vars setup and having a single source of truth for building.
If we want to avoid go-mod tidy and linting we can probably add a target without those dependencies?
Although I don't think it adds a lot of latency to the build (the go generate probably takes most of the time anyway)
There was a problem hiding this comment.
I prefer we avoid the circular dependency of make calling this and then this calling make.
There was a problem hiding this comment.
I think I agree with @RonFed, calling make build reduces the need to make sure changes are duplicated
There was a problem hiding this comment.
Adding a one-off target to the Makefile just to run these commands does not seem appropriate to me.
It is a one-off command that is only ever meant to be run by the Dockerfile. Why not just add the command to the Dockerfile?
Additionally, it is adding the command to a development tooling file, but it is not meant to be run by developers. What happens when we get an issue asking why the new make target fails because their repo was not correctly linted before-hand? That seems like a valid complaint. They are running developer tooling, its just they don't have the domain history of the project to know that this "one special target" isn't meant for them to run.
Running the make target also means that we will need to copy over the Makefile. This, again, will mean that unrelated changes to the repo (the Makefile) will invalidate the docker cache. This happens a lot in developement.
I don't see adding a new include directory to BPF2GO_CFLAGS in both the Makefile and the Dockerfile as a burden given how little this happens.
There was a problem hiding this comment.
Calling make build from the dockerfile is common for the reasons Ron and I mentioned. But, this isn't a huge deal
There was a problem hiding this comment.
Additionally, it is adding the command to a development tooling file, but it is not meant to be run by developers. What happens when we get an issue asking why the new make target fails because their repo was not correctly linted before-hand? That seems like a valid complaint. They are running developer tooling, its just they don't have the domain history of the project to know that this "one special target" isn't meant for them to run.
This can be addressed by improving the Makefile.
For example when the build target is triggered, it can check if it is running on linux, otherwise return an informative error.
There was a problem hiding this comment.
@RonFed I don't follow. I develop on a Linux system, why would I want it to succeed for me but not for a Mac dev?
Also, that wouldn't address the docker cache invalidation I am trying to address.
| GOARCH=$TARGETARCH \ | ||
| CGO_ENABLED=$CGO_ENABLED \ | ||
| BPF2GO_CFLAGS=$BPF2GO_CFLAGS \ | ||
| go generate ./... \ | ||
| && go build -o otel-go-instrumentation ./cli/... |
There was a problem hiding this comment.
Calling make build from the dockerfile is common for the reasons Ron and I mentioned. But, this isn't a huge deal
Stop copying the whole project when building the auto docker image. This causes unrelated changes to the parts of the codebase to invalidate the docker cache This also stops using the `make` utility to build the project. Doing so brings in many steps outside of building the target binary (i.e. linting and running `go mod tidy`). Instead, run the go commands that are needed directly.
Stop copying the whole project when building the auto docker image. This causes unrelated changes to the parts of the codebase to invalidate the docker cache
This also stops using the
makeutility to build the project. Doing so brings in many steps outside of building the target binary (i.e. linting and runninggo mod tidy). Instead, run the go commands that are needed directly.